Porting: auth module #3
Conversation
* resolved todos in keeper.go (except 1) * updated x/auth/keeper/grpc_query.go with comments * added comments in auth/module.go * removed depinject from auth.ModuleInputs * resolved TODOs in auth/keeper/genesis (created NewBaseAccount and passed to processor) * removed some TODOs from x/auth/types/account.go * added few comments in x/auth/ante/ante.go * few final comments and modifications
…nal txBuilder tests
|
@marcello33 your pull request is missing a changelog! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| x.Key = value.Bytes() | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.crypto.secp256k1.PubKeyOld")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.crypto.secp256k1.PubKeyOld")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.crypto.secp256k1.PubKeyOld does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| x.Key = value.Bytes() | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.crypto.secp256k1.PubKey")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.crypto.secp256k1.PubKey")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.crypto.secp256k1.PubKey does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| x.Key = value.Bytes() | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.crypto.secp256k1.PubKey")) | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.crypto.secp256k1.PrivKeyOld")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| } | ||
| panic(fmt.Errorf("message cosmos.crypto.secp256k1.PubKey does not contain field %s", fd.FullName())) | ||
| panic(fmt.Errorf("message cosmos.crypto.secp256k1.PrivKeyOld does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| func (pubKey *PubKey) Address() crypto.Address { | ||
| if len(pubKey.Key) != PubKeySize { | ||
| panic("length of pubkey is incorrect") | ||
| panic(fmt.Sprintf("length of pubkey is incorrect %d != %d", len(pubKey.Key), PubKeySize)) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| @@ -164,7 +164,7 @@ | |||
| // in SDK except in a tendermint validator context. | |||
| func (pubKey *PubKey) Address() crypto.Address { | |||
| if len(pubKey.Key) != PubKeySize { | |||
| panic("pubkey is incorrect size") | |||
| panic(fmt.Sprintf("length of pubkey is incorrect %d != %d", len(pubKey.Key), PubKeySize)) | |||
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| // Address returns an ethereum style addresses | ||
| func (pubKey *PubKeyOld) Address() crypto.Address { | ||
| if len(pubKey.Key) != PubKeySize { | ||
| panic(fmt.Sprintf("length of pubkey is incorrect %d != %d", len(pubKey.Key), PubKeySize)) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if len(strings.TrimSpace(text)) == 0 { | ||
| return []byte{}, errors.New("empty address string is not allowed") | ||
| } | ||
|
|
There was a problem hiding this comment.
Should we make a check for exact length of address?
There was a problem hiding this comment.
That is done already by VerifyAddressFormat, specifically by common.IsHexAddress
| if len(bz) == 0 || bz == nil { | ||
| return "", nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Can we also check for bytes length?
There was a problem hiding this comment.
This would not be useful, as - once translated to string - it will be done by VerifyAddressFormat, specifically by common.IsHexAddress
| @@ -10,7 +10,7 @@ import ( | |||
|
|
|||
| "github.com/cometbft/cometbft/crypto" | |||
| secp256k1 "github.com/decred/dcrd/dcrec/secp256k1/v4" | |||
| "golang.org/x/crypto/ripemd160" //nolint: staticcheck // keep around for backwards compatibility | |||
| ethCrypto "github.com/ethereum/go-ethereum/crypto" //nolint:depguard | |||
|
|
|||
There was a problem hiding this comment.
Shouldn't we keep this for easy upstream or no need?
There was a problem hiding this comment.
ripemd160 is no longer used, so no need to keep it.
ethCrypto is there of course.
| @@ -0,0 +1,50 @@ | |||
| package codec | |||
|
|
|||
There was a problem hiding this comment.
Why this file is defined again?
Can't we the Hexcodex we defined earlier?
There was a problem hiding this comment.
To keep our fork as close as possible to upstream.
It is exactly the same in cosmos, where bech32 codec is defined in the codec package, but also at modules' level
| anteDecorators := []sdk.AnteDecorator{ | ||
| ante.NewSetUpContextDecorator(), // outermost AnteDecorator. SetUpContext must be called first | ||
| circuitante.NewCircuitBreakerDecorator(options.CircuitKeeper), | ||
| ante.NewExtensionOptionsDecorator(options.ExtensionOptionChecker), | ||
| ante.NewValidateBasicDecorator(), | ||
| ante.NewTxTimeoutHeightDecorator(), | ||
| // ante.NewTxTimeoutHeightDecorator(), |
There was a problem hiding this comment.
Why we removed this? Though it is not there in Heimdall but can it be useful to us?
There was a problem hiding this comment.
With the purpose of only preserving only the heimdall changes, I removed everything which was not there.
Maybe we can investigate this functionality and enable it afterwards. Wdyt? In case, I'll create a JIRA task.
| NewTxTimeoutHeightDecorator(), | ||
| NewValidateMemoDecorator(options.AccountKeeper), | ||
| // NewTxTimeoutHeightDecorator(), // HV2: this is not present in heimdall. Is it safe to remove? | ||
| NewValidateMemoDecorator(options.AccountKeeper), // TODO HV2: can we keep this despite we don't support Memo? Or is it better/safer to remove it? |
There was a problem hiding this comment.
Memo don't have any impact on tx logic, right? But it may increase the size of transaction.
There was a problem hiding this comment.
Right. But I checked, and it's in heimdall-v1 too, that's why I kept it.
x/auth/ante/setup.go
Outdated
| // if ctx.BlockHeight() < helper.GetAalborgHardForkHeight() && (stdTx.Msg.Type() == checkpointTypes.EventTypeMilestone || stdTx.Msg.Type() == checkpointTypes.EventTypeMilestoneTimeout) { | ||
| // newCtx = SetGasMeter(simulate, ctx, 0) | ||
| // return newCtx, sdk.ErrTxDecode("error decoding transaction").Result(), true | ||
| // } |
There was a problem hiding this comment.
We can remove this comment, any hard fork is not required as we are starting a new chain.
* add: port gov module * chg: implement WeightedVoteOptions with constraints * chg: better TODOs descriptions * chg: POS-2135: fix some tests * chg: POS-2135: fix more tests * chg: POS-2135: update an address format * chg: fix few more tests * chg: fix few more tests * chg: fix some tests / temp revert some others to properly tune params later on * chg: fix TestHooks * chg: fix burn related methods / fix tests * chg: fix query for WeightedVoteOptions / better comments * chg: fix all tests in gov module * chg: fix a staking integration test * chg: POS-2142: edit gov readme * chg: use AccAddressFromHex in tests instead of addressCodec * chg: enable one test / provide better context for the only skipped test * chg: use hex acc addresses in gov tests * chg: return empty string on ProposalType normalization * chg: remove TextProposals / add comment for Msgs auto-execution * chg: re-enable textProposals / TBD with team * chg: remove comment * chg: better context for HV2 TODOs * chg: filter out non valid proposals msgs types and content * chg: fix typeUrls * chg: filter out not supported messages at time of proposals submit / fix tests accordingly * chg: go mod tidy * chg: address PR comments: filtering dedicated file / test msg types / edit comments * chg: register interfaces in gov test app * chg: better context for comments * chg: comment for future improvements * chg: consistent example of gov tx for submit proposal * chg: add msgServers in testApp to allow additional MsgUpdateParams types * chg: fix tests after merge * chg: update go deps for sdk and simapp * chg: comment * chg: comment in README for further actions --------- Co-authored-by: Raneet Debnath <[email protected]>
* proto,x/bank: initial port of heimdall related changes for bank module * x/bank: rm moduleName param from SubtractCoins + rm MsgSetSendEnabled,add MsgMultiSend to amino registry + rm unused commented code * simapp,x/auth,x/bank: address PR comments * x/bank: change feeAmount to defaultFeeAmount in test * x/bank: address PR comments * x/bank: use CreateRandomValidatorSet instead of manually initialising validator set in tests * x/bank,proto: change heimdall v2 comment format * all: fix broken simapp dep * x/bank: rm SetCoins * x/auth,x/bank: modify bank tests with assertions for fee_collector and account balances * x/bank: minor code refactors
This PR is a first attempt to port the
authmodule implementation fromheimdalltocosmos-sdk.Please, pay particular attention to the comments starting with prefix
TODO HV2, as they define open points that need action within this PR, or later on (once other modules are implemented).I would recommend reviewing everything under
x/auth...first, as there are also editions to other modules/packages ofcosmos-sdk.Such changes are made with the only purpose to be able to build the overall project.
For example, wrt the change from
bech32tohex, the tests have been fixed inauthmodule, but it may happen that in other modules you still seebech32prefixes/addresses being used, despite the underlying codec/methods arehexbased. Those occurrences/tests will be fixed when such modules will be considered for implementation/porting, or at the end of the migration process.